Skip to content

[Sol->Yul] Provide selector for some internal functions. #11015

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 25, 2021

Depends on #11014
References #10905
Closes #10891

@chriseth chriseth requested a review from cameel February 25, 2021 10:56
@bshastry
Copy link
Contributor

Closes #10891

@cameel cameel force-pushed the fixConstantsCallGraph branch from 5dc0dce to 186d14d Compare February 25, 2021 15:01
@chriseth chriseth force-pushed the fixSelectorForInternal branch from 909a59a to 814fdca Compare February 25, 2021 15:04
Comment on lines 1 to 7
contract B {
function g() public {}
}
contract C is B {
bytes4 constant s2 = B.g.selector;
function f() external pure returns (bytes4) { return s2; }
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add more cases from #10891, maybe combined into a single test:

contract B {
    function ext() external {}
    function pub() public {}
}

contract C is B {
    function test() public {
        B.ext.selector;
        B.pub.selector;
        this.ext.selector;
        pub.selector;
    }
}

contract D {
    function test() public {
        B.ext.selector;
        B.pub.selector;
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that those cannot be done as a semantic test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps split into two tests

contract B {
    function ext() external {}
    function pub() public {}
}

contract C is B {
    function test() public returns (bytes4, bytes4, bytes4, bytes4) {
        return (B.ext.selector, B.pub.selector, this.ext.selector, pub.selector);
    }
}
// ====
// compileViaYul: true
// ----
// test() ->

and

contract B {
    function ext() external {}
    function pub() public {}
}

contract D {
    function test() public returns (bytes4, bytes4) {
        return (B.ext.selector, B.pub.selector);
    }
}
// ====
// compileViaYul: true
// ----
// test() -> 0xcf9f23b500000000000000000000000000000000000000000000000000000000, 0x7defb41000000000000000000000000000000000000000000000000000000000

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the tuple really trigger the same code path? I'll add a syntax test for the others just to be sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the tuple really trigger the same code path? I'll add a syntax test for the others just to be sure.

Yes, just verified that it does.

Base automatically changed from fixConstantsCallGraph to develop February 25, 2021 16:05
@chriseth chriseth force-pushed the fixSelectorForInternal branch 2 times, most recently from 6ef8363 to 9e44b1c Compare March 1, 2021 13:49
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected this to be more complicated, but looks to me like it should be fine. More tests won't hurt, though. Also should this get a changelog entry?

function g() public {}
}
contract C is B {
bytes4 constant s2 = B.g.selector;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this exactly the case that already worked before?

@chriseth
Copy link
Contributor Author

chriseth commented Mar 1, 2021

No changelog entry because it is sol->yul

@chriseth chriseth force-pushed the fixSelectorForInternal branch from 9e44b1c to 5e94fce Compare March 1, 2021 15:20
@chriseth
Copy link
Contributor Author

chriseth commented Mar 1, 2021

Added tests.

@@ -0,0 +1,26 @@
contract B {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file does not trigger an error when run through solc --ir (and I think this is also tested for all syntax tests)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we run IR generator on all syntax tests? That's good to know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acutally we do not. We compile to bytecode, but not via yul. I just tried it and it is kind of stuck in the optimizer :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we run IR generator on all syntax tests? That's good to know.

Not yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just ran the IR on all syntax tests and found one unimplemented feature: #11024

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did that a few weeks ago already (it used to be more like 9 cases or so), so that issue may be a duplicate...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird... I do remember that we were wondering if it actually works and that I noted that it's a bit weird even in old codegen... but maybe we still didn't do anything about it or even file an issue :-).

Copy link
Contributor

@bshastry bshastry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chriseth chriseth merged commit ce5c597 into develop Mar 2, 2021
@chriseth chriseth deleted the fixSelectorForInternal branch March 2, 2021 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[soltoyul] ICE due to invalid use of selector function
5 participants